-
Notifications
You must be signed in to change notification settings - Fork 280
transpile: Always use absolute paths when referring to crates #1408
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
kkysen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the CI failings are because you didn't update the os- and arch-specific snapshots that aren't run locally.
| )] | ||
| #[no_mangle] | ||
| pub static mut TRUE: core::ffi::c_int = 1 as core::ffi::c_int; | ||
| pub static mut TRUE: ::core::ffi::c_int = 1 as ::core::ffi::c_int; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ahomescu, is #1399 able to handle stuff like this and still shorten the paths with imports? Also, is it or another refactor able to remove the :: where not needed? Would that be difficult at all?
The change here is correct and handles some edge cases, but it is longer, so it'd be nice to be able to refactor out the unnecessary ones afterward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, I can give it a try. I'm not sure you can run reorganize_definitions multiple times on the same code base, but we can find out.
71c6b98 to
8d87265
Compare
The three snapshot CI runs are failing because the leading double colon I can find only one place in the whole codebase that adds an import with the string "asm":
All items added to the item store are eventually fed through c2rust/c2rust-transpile/src/rust_ast/item_store.rs Lines 55 to 64 in 8d87265
And yet, the generated code does not have an absolute path. It's as if |
kkysen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is weird. fs::read_to_string(&platform_rs_path).unwrap() is showing the use ::core::arch::asm;, but then the fs::read_to_string(&rs_path).unwrap() a few lines later is showing use core::arch::asm; again.
|
Ah, I figured out what it is. It's |
400a962 to
c9a70be
Compare
kkysen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
It seems to be fixed, but it is now getting stuck on the empty |
This is what was breaking #1408 (comment). @Rua, I think this should fix your issue. Let me know and you can review this PR.
|
Ugh, why did this get closed? |
GH seems to suggest it was closed because the target branch was deleted. |
Ugh, GitHub being dumb again. Normally when you delete the target branch, it changes it to |
2b806c0 to
d41596d
Compare
|
Hmm strange, the CI is still failing on the same error. Is it perhaps using an old version? EDIT: But the MacOS test passed... huh? |
I'm not sure, but the error message is slightly different. Might be a different error? |
|
Transpile is able to generate a new Cargo.toml when transpiling, and it will fill it with dependencies used by the transpiled code, including |
kkysen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Transpile is able to generate a new Cargo.toml when transpiling, and it will fill it with dependencies used by the transpiled code, including
c2rust-bitfields. Is it possible that the file it generates there refers to the stable/crates.io version ofc2rust-bitfields, and not the one in the current directory tree? If so, that could explain what's going on maybe.
I think you're right. And it seems to be using c2rust-bitfields= "0.3" from what I can tell, which is very old.
kkysen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…of c2rust crates are used (#1420) The `transpile.gen.sh`s generated by `c2rust-testsuite` use `C2RUST_DIR` to set dependencies in generated `Cargo.toml`s to point to the current `c2rust` directory, allowing it to test with the current version of `c2rust-bitfields` and `c2rust-asm-casts`. This needs #1419 fixed first, and it should unblock #1408, which depends on the `c2rust-bitfields-derive` fix in #1411.
d2f9a1e to
9c8e078
Compare
|
This is now passing the tests, but the Clang 15 test seems to be failing for an unrelated reason. EDIT: Seems to be fixed again. |
libc::c_intshould be globally qualified like::libc::c_intto avoid name clashes #447.